-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sanitize param usage in some plumbing task script blocks #977
Conversation
Prior to this commit we had several script blocks using the syntax `$(params.foo)` to pull in param values. As per #971 it is advised to pull these usages into environment variables to improve robustness (unexpected characters in param values like quotes and ampersands can cause confusing issues when a script runs) and to remove one avenue of script injection in scenarios when a param includes user-submitted content. This PR patches resources in the cd, images, nightly-release, nightly-tests and release/base directories to move param referencing into environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @sbwsg!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
/kind misc |
/test check-pr-has-kind-label |
env: | ||
- name: PACKAGE | ||
value: $(params.package) | ||
- name: VERSION | ||
value: $(params.version) | ||
- name: EXTRA_FILE | ||
value: $(params.extra-file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot have an env
directly under spec
, it must either go into steps
or into a stepTemplate
.
env: | ||
- name: REMOTE_HOST | ||
value: $(params.remote-host) | ||
- name: REMOTE_USER | ||
value: $(params.remote-user) | ||
- name: REMOTE_PORT | ||
value: $(params.remote-port) | ||
- name: ACTION | ||
value: $(params.action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
env: | ||
- name: PACKAGE | ||
value: $(params.package) | ||
- name: VERSION_TAG | ||
value: $(params.versionTag) | ||
- name: RELEASE_BUCKET | ||
value: $(params.releaseBucket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Changes
Prior to this commit we had several script blocks using the syntax
$(params.foo)
to pull in param values. As per #971it is advised to pull these usages into environment variables to
improve robustness (unexpected characters in param values like quotes and ampersands
can cause confusing issues when a script runs) and to remove
one avenue of script injection in scenarios when a param includes
user-submitted content.
This PR patches resources in the cd, images, nightly-release,
nightly-tests and release/base directories to move param referencing
into environment variables.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.